Review — GLM 5.2

Reviewed PR #115 (feat: add quantus exercise chain smoke/fuzz suite) by checking out the branch locally, reading every changed source file, and running the toolchain.

Verification performed

• cargo check --release --locked → clean compile (48s, exit 0; only a pre-existing trie-db future-incompat note from a transitive dep)
• cargo clippy --release --locked → no lints (only build-script info from ZK circuit generation)
• No dangling references to the deleted referenda / referenda_decode modules or the removed SchedulerCommands::ScheduleRemark / submit_scheduler_* stubs
• Cross-checked every reused helper signature against its definition (transfer_with_nonce, submit_transaction_with_nonce, batch_transfer, submit_preimage, predict_multisig_address, get_multisig_info, list_proposals, vote_on_referendum, is_member, …) — all match
• Confirmed rand = "0.9" so random() / random_range() usage is correct
• Confirmed dev wallets are encrypted with "" → matches the wormhole phase's Some(String::new())
• Confirmed .github/actions/ubuntu composite action exists; config compat table bumped 131 → 134 matching the regenerated metadata

Strengths

• Negative scenarios pin specific expected errors (FundsUnavailable, BadOrigin, DelayTooShort, AccountNotHighSecurity, GuardianCannotBeSelf, NotRecoverable) — exactly the right design for a pre-upgrade regression gate: an upgrade that loosens validation fails the suite instead of silently passing.
• Metadata canaries (scheduler_calls_disabled, removed_pallets_absent) assert Scheduler stays dispatch-disabled and Referenda/ConvictionVoting/Sudo stay absent. Clever, low-cost regression detection.
• Fuzz invariant is well-scoped: clean success or clean rejection both pass; only hangs / undecodable failures fail. Boundary amounts (0, 1, ED±1, u128::MAX) hit the interesting edges.
• QuietGuard RAII restores logging on every exit path; --verbose correctly overrides quiet.
• Security: commit 4ef3f49 removed the mnemonic / secret / secret-prefix logging that CodeQL flagged.
• Good reuse of existing CLI handlers (send::transfer, multisig::*, tech_collective::*, wormhole multiround) — no parallel implementations. build_submit_call is shared between governance and upgrade scenarios (DRY).
• ephemeral_accounts.max(4) guarantees eph[0..3] indexing can't panic; transfer_below_ed guards ED <= 1; stale_nonce guards current_nonce == 0.
• Upgrade phase reconnects the client to pick up new metadata and includes the referendum state in the failure message for debugging.
• CI workflow uses if: always() for artifact upload and node cleanup, has an RPC warmup loop, and a generous 180-min timeout.

Non-blocking notes / nits

1. reads.rs:19 naming mismatch — the scheduler_agenda step actually calls get_last_processed_timestamp, not the agenda. Cosmetic, but misleading in the JSON report.
2. DRY: referendum submission is duplicated between governance.rs::referendum_flow and upgrade.rs::governance_set_code (note preimage → submit → decision deposit → 3× vote). A shared submit_referendum_with_votes(ctx, encoded, hash, len) -> Result<u32> helper would let upgrade just add polling + refund on top. Aligns with the repo's DRY rule.
3. upgrade.rs refund timing — the deposit refunds are submitted via ctx.client before the caller reconnects, i.e. with the pre-upgrade metadata. If a future upgrade changes tech_referenda call indices, the refund would fail to decode. It's best-effort (doesn't fail the step), so non-blocking, but worth a one-line comment or moving the refund after reconnect.
4. fuzz.rs classify heuristic is broad — any error containing "Transaction … failed" / "invalid" / "error" / "dropped" / "Failed to submit" passes. Intentional for the stated invariant (catch hangs / streams ending) and partly documented, but it means a novel undecodable error that happens to contain "error" would still pass. Worth a sentence in the doc comment calling out that trade-off.
5. governance.rs::referendum_flow assumes bob + charlie are tech-collective members (votes with all three, asserts ≥3 ayes) but membership_reads only asserts alice is a member. If dev genesis ever ships only alice, the bob/charlie votes would fail at dispatch with a less obvious error. Asserting all three memberships in membership_reads would make the dependency explicit and fail fast.
6. CI: actions/checkout of Quantus-Network/chain — the workflow's GITHUB_TOKEN is scoped to the cli repo (contents: read). If chain is private, that checkout will fail to authenticate. If it's public, no issue — worth confirming.
7. CodeQL alerts on this PR are non-issues: the "hard-coded cryptographic value" on nonce 0 in stale_nonce is an intentional stale nonce, not a secret; the "cleartext logging" hits are public on-chain data (addresses, nonces, balances). The one real secret-logging path was fixed in 4ef3f49.

Verdict

Approve. The change compiles cleanly, passes clippy, removes dead code (referenda modules, scheduler dispatch stubs) without leaving dangling references, and the one real security finding was addressed. The design is strong — especially the pinned-error negative scenarios and the metadata canaries, which are exactly what you want around a runtime upgrade. The remaining items are nits / non-blocking notes. The only residual risk is the upgrade phase being marked not-yet-exercised end-to-end, which is clearly documented in the PR body and test plan.

Reviewed the branch locally at 4ef3f49: cargo build --locked, cargo clippy --all-targets --locked, and cargo test --locked (210 passed) are all clean, and all 9 CI checks are green. The overall design is strong — scenarios reuse the real CLI handlers so the suite tests the actual code paths, negative scenarios pin specific expected errors, and the scheduler-disabled / removed-pallet canaries are exactly the kind of thing that catches an upgrade silently loosening validation. The referenda/scheduler removal left no dangling references, and the wormhole secret-logging cleanup addresses the CodeQL findings.

Should fix

1. The upgrade phase cannot fully pass with a single-entry compatibility table.
reads::runtime_version fails unless is_runtime_compatible(spec, tx), and COMPATIBLE_RUNTIMES pins exactly {spec: 134, tx: 2}. The upgrade phase requires the WASM's spec_version to be higher than the current one (it polls for the bump), and the post-upgrade re-run starts with post-upgrade:reads / runtime_version against that new spec — which by construction is not in the table. So a successful upgrade still records a failure and the suite exits nonzero. (Flipping the table to the new spec instead just moves the failure to the pre-upgrade pass.) The table already supports multiple entries, so the fix is either documenting "add the candidate spec to COMPATIBLE_RUNTIMES before an --upgrade-wasm run" or special-casing the check in the post-upgrade pass. As written, the first real upgrade-gate run will report red regardless of outcome — consistent with the PR's own "not yet exercised end-to-end" caveat, but worth fixing before relying on it.

2. --seed reproduction fails setup on a non-fresh chain.
Ephemeral keypairs derive deterministically from the seed, and fund_ephemeral_accounts asserts the post-funding balance is exactly 1000 tokens. Re-running with the same seed against the same chain — the exact "reproduce this fuzz failure" workflow the seed exists for — hits leftover balances from the previous run and dies in setup. Checking the balance delta instead would fix it.

Nits

• args.ephemeral_accounts.max(4) silently overrides the flag. The minimum is real (scenarios index eph[0..=3]), so either document it in the help text or reject values below 4.
• stale_nonce passes on any error containing "Invalid", which is broad enough to mask an unrelated rejection.
• Fuzz recipients are fresh throwaway accounts 1/3 of the time, so funds steadily leak out of the ephemeral set; with a large --fuzz-iterations the post-upgrade balances pass could start failing on FundsUnavailable. Fine at the default 25.
• exercise.yml builds the CLI with SKIP_CIRCUIT_BUILD=1, so wormhole circuit generation happens at runtime inside the suite (via ensure_bins_dir). It works, but it silently inflates the wormhole phase timing on every cold CI run.
• With --json, the report is printed after the human-readable status lines and CI tees both into one artifact, so anything consuming the JSON has to strip the preamble first.
• The wormhole phase was promoted into the default set (at 5×5) after the stated "full suite green" run, so the first nightly is effectively its validation run.

One thing that looked like a bug but isn't: the post-upgrade governance re-run re-notes a deterministic preimage (same treasury portion → same hash), but submit_preimage treats AlreadyNoted as success, so the rerun is safe. Likewise the QuietGuard drop impl correctly restores logging on every exit path.

Verdict

Approve with minor fixes. The default nightly path (all phases except upgrade) is sound and CI-ready as-is. #1 should be addressed — in code or docs — before the upgrade phase is used as the real pre-upgrade gate, and #2 is worth fixing so --seed delivers the reproducibility it advertises. Neither blocks merging the suite itself.

These should be fixed

1. The upgrade phase cannot fully pass with a single-entry compatibility table.
   reads::runtime_version fails unless is_runtime_compatible(spec, tx), and COMPATIBLE_RUNTIMES pins exactly {spec: 134, tx: 2}. The upgrade phase requires the WASM's spec_version to be higher than the current one (it polls for the bump), and the post-upgrade re-run starts with post-upgrade:reads / runtime_version against that new spec — which by construction is not in the table. So a successful upgrade still records a failure and the suite exits nonzero. (Flipping the table to the new spec instead just moves the failure to the pre-upgrade pass.) The table already supports multiple entries, so the fix is either documenting "add the candidate spec to COMPATIBLE_RUNTIMES before an --upgrade-wasm run" or special-casing the check in the post-upgrade pass. As written, the first real upgrade-gate run will report red regardless of outcome — consistent with the PR's own "not yet exercised end-to-end" caveat, but worth fixing before relying on it.

2. --seed reproduction fails setup on a non-fresh chain.
   Ephemeral keypairs derive deterministically from the seed, and fund_ephemeral_accounts asserts the post-funding balance is exactly 1000 tokens. Re-running with the same seed against the same chain — the exact "reproduce this fuzz failure" workflow the seed exists for — hits leftover balances from the previous run and dies in setup. Checking the balance delta instead would fix it.

3 - How is upgrade supposed to work if the time is set to 24h voting?